Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

swarm / rcmgr: synchronize the concurrent outbound dials with limits #1898

Merged
merged 2 commits into from
Nov 22, 2022

Conversation

MarcoPolo
Copy link
Collaborator

We see flakiness in CI because a client will dial with concurrency 8 but the limit on the server is 4 so we get rcmgr failures. We notice this when we have a lot of addrs to dial.

Fixes #1816
Fixes #1765
Fixes #1730
Fixes #1668

(will rebase to master after #1881 is merged)

@marten-seemann
Copy link
Contributor

(will rebase to master after #1881 is merged)

Why not target master from this PR? Otherwise this commit will be lost when we squash #1881.

@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 0b7bfc7 to 65a2a55 Compare November 17, 2022 05:22
@MarcoPolo
Copy link
Collaborator Author

(will rebase to master after #1881 is merged)

Why not target master from this PR? Otherwise this commit will be lost when we squash #1881.

To make it easier to review. I'll merge this to master so we'll keep this commit. I should have said target master after #1881

@MarcoPolo MarcoPolo changed the base branch from marco/quic-v1 to master November 17, 2022 18:01
@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 65a2a55 to 92d131b Compare November 17, 2022 18:02
@MarcoPolo
Copy link
Collaborator Author

Oh I see what you meant. This PR didn't build on the quic-v1 branch so there was no point in targeting that branch. I just defaulted to stacking it since it came from that branch, but I should have just based this on master from the start.

@marten-seemann marten-seemann changed the title Synchronize the concurrent outbound dials with limits swarm / rcmgr: synchronize the concurrent outbound dials with limits Nov 21, 2022
@MarcoPolo MarcoPolo force-pushed the marco/synchronize-limits-with-dials branch from 54432e3 to aef3657 Compare November 22, 2022 21:50
@MarcoPolo
Copy link
Collaborator Author

@marten-seemann and I talked about this and agreed to bump the limit up to 8 for now. This will be handled better when we have smart dialing.

@MarcoPolo MarcoPolo mentioned this pull request Nov 22, 2022
34 tasks
@@ -492,7 +492,7 @@ var DefaultLimits = ScalingLimitConfig{
},

PeerBaseLimit: BaseLimit{
ConnsInbound: 4,
ConnsInbound: 8,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment explaining why this is 8 for now, and that smart dialing will allow us to reduce this back to 4?

@MarcoPolo MarcoPolo merged commit 9c9122d into master Nov 22, 2022
@p-shahi p-shahi mentioned this pull request Nov 22, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
2 participants